Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Updating the JIT to take EnableSSE3_4 into account when setting the supported instruction sets #16395

Merged
merged 1 commit into from
Feb 16, 2018
Merged

Updating the JIT to take EnableSSE3_4 into account when setting the supported instruction sets #16395

merged 1 commit into from
Feb 16, 2018

Conversation

tannergooding
Copy link
Member

Resolves the error brought up here: #16378 (comment)

This updates the setSupportedISA calls to take JitConfig.EnableSSE3_4 into account. This also removes the compCanUseSSE4 field in favor of more explicit compSupports calls.

FYI. @fiigii, @CarolEidt, @AndyAyersMS

tannergooding
tannergooding commented Feb 15, 2018
View reviewed changes
src/jit/compiler.cpp Outdated
@@ -2716,7 +2710,7 @@ void Compiler::compSetProcessor()
codeGen->getEmitter()->SetContainsAVX(false);
codeGen->getEmitter()->SetContains256bitAVX(false);
}
else if (CanUseSSE4())
else if (compSupports(InstructionSet_SSE41) || compSupports(InstructionSet_SSE42))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to support emitting the 4-byte encoding for both SSE4.1 and SSE4.2 instructions, even if only one of them is enabled.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the original behavior is compSupports(InstructionSet_SSE41) && compSupports(InstructionSet_SSE42))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original behavior will be broken for the case where EnableSSE42=1 and EnableSSE41=0, which is completely valid to set today

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. This is setting the bit in emitter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to also include InstructionSet_SSSE3, which also uses the 0F 38 and 0F 3A encoding.

tannergooding
tannergooding commented Feb 15, 2018
View reviewed changes
src/jit/compiler.h Outdated
@@ -7462,7 +7462,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
return SIMD_AVX2_Supported;
}

if (CanUseSSE4())
if (compSupports(InstructionSet_SSE41))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SIMD only emits SSE4.1 instructions

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, the legacy SIMD code can generate pcmpgtq that is a SSE4.2 instruction.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update. I forgot that instruction was 4.2

tannergooding
tannergooding commented Feb 15, 2018
View reviewed changes
src/jit/compiler.cpp Outdated
@@ -2666,47 +2666,41 @@ void Compiler::compSetProcessor()
opts.setSupportedISA(InstructionSet_POPCNT);
}
}
if (jitFlags.IsSet(JitFlags::JIT_FLAG_USE_SSE3))

if (JitConfig.EnableSSE3_4())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only mark SSE3, SSSE3, SSE4.1, and SSE4.2 as enabled if both the EnableSSE3_4 flag and the individual ISA flag is set.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@tannergooding
Copy link
Member Author

Updated and added explicit comments.

tannergooding
tannergooding commented Feb 15, 2018
View reviewed changes
src/jit/compiler.cpp Outdated
@@ -2716,8 +2713,11 @@ void Compiler::compSetProcessor()
codeGen->getEmitter()->SetContainsAVX(false);
codeGen->getEmitter()->SetContains256bitAVX(false);
}
else if (CanUseSSE4())
else if (compSupports(InstructionSet_SSE41) || compSupports(InstructionSet_SSE42))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could create a compSupportsAny(InstructionSet_SSE41 | InstructionSet_SSE42) function to simplify this, if we think it is worthwhile.

tannergooding
tannergooding commented Feb 15, 2018
View reviewed changes
src/jit/compiler.h
if (CanUseSSE4())
// SIMD_SSE4_Supported actually requires all of SSE3, SSSE3, SSE4.1, and SSE4.2
// to be supported. We can only enable it if all four are enabled in the compiler
if (compSupports(InstructionSet_SSE42) && compSupports(InstructionSet_SSE41) &&
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could create a compSupportsAll(InstructionSet_SSE3 | InstructionSet_SSSE3 | InstructionSet_SSE41 | InstructionSet_SSE42) function to simplify this, if we think it is worthwhile.

@tannergooding
Copy link
Member Author

CC. @dotnet/jit-contrib

CarolEidt
CarolEidt approved these changes Feb 16, 2018
View reviewed changes
Copy link

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/jit/compiler.cpp
{
// Emitter::UseSSE4 controls whether we support the 4-byte encoding for certain
// instructions. We need to check if either is supported independently, since
// it is currently possible to enable/disable them separately.
codeGen->getEmitter()->SetUseSSE4(true);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to consider changing the name of this flag, as this is a bit confusing. But I don't think it's critical.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I'll log a bug to track it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/dotnet/coreclr/issues/16421

@tannergooding
Copy link
Member Author

Applied formatting patch.

@tannergooding
Copy link
Member Author

test Windows_NT x64 Checked jitsse2only

test Windows_NT x64 Checked jitincompletehwintrinsic
test Windows_NT x64 Checked jitx86hwintrinsicnoavx
test Windows_NT x64 Checked jitx86hwintrinsicnoavx2
test Windows_NT x64 Checked jitx86hwintrinsicnosimd
test Windows_NT x64 Checked jitnox86hwintrinsic

test Windows_NT x86 Checked jitincompletehwintrinsic
test Windows_NT x86 Checked jitx86hwintrinsicnoavx
test Windows_NT x86 Checked jitx86hwintrinsicnoavx2
test Windows_NT x86 Checked jitx86hwintrinsicnosimd
test Windows_NT x86 Checked jitnox86hwintrinsic

test Ubuntu x64 Checked jitincompletehwintrinsic
test Ubuntu x64 Checked jitx86hwintrinsicnoavx
test Ubuntu x64 Checked jitx86hwintrinsicnoavx2
test Ubuntu x64 Checked jitx86hwintrinsicnosimd
test Ubuntu x64 Checked jitnox86hwintrinsic

test OSX10.12 x64 Checked jitincompletehwintrinsic
test OSX10.12 x64 Checked jitx86hwintrinsicnoavx
test OSX10.12 x64 Checked jitx86hwintrinsicnoavx2
test OSX10.12 x64 Checked jitx86hwintrinsicnosimd
test OSX10.12 x64 Checked jitnox86hwintrinsic

@tannergooding tannergooding merged commit 8a87e42 into dotnet:master Feb 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fiigii fiigii fiigii left review comments

@CarolEidt CarolEidt CarolEidt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tannergooding @fiigii @CarolEidt